perf: optimize regexp_instr (40% faster)#23540
Conversation
andygrove
changed the title
regexp_instr (40% faster)
| }; | ||
| let pattern: &Regex = compile_and_cache_regex(pattern, flags, regex_cache)?; | ||
| // println!("get_index: value = {}, pattern = {}, start = {}, n = {}, subexpr = {}, flags = {:?}", value, pattern, start, n, subexpr, flags); | ||
| ) -> Result<i64, ArrowError> { |
There was a problem hiding this comment.
should it return -1 if there is no match? 0 sounds like legit index
There was a problem hiding this comment.
This PR doesn't make any functional changes. The index is 1-based, I believe.
| } | ||
|
|
||
| fn criterion_benchmark(c: &mut Criterion) { | ||
| let size = 1024; |
There was a problem hiding this comment.
I think most batches are 8k not 1k
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @andygrove -- this looks good to me
While reviewing I also found a few other cases that I think are worth testing; I made a separate PR for them here:
| use std::hint::black_box; | ||
| use std::sync::Arc; | ||
|
|
||
| fn create_args<O: OffsetSizeTrait>( |
There was a problem hiding this comment.
FWIW @andygrove -- it would make it easier to verify these results if you made one PR to add the benchmark and then a second PR to make the code change
That way we can use the run benchmark <benchmark> bot command to get a second performance run
| Ok(Some(match_start_char_offset)) | ||
| } else { | ||
| Ok(Some(0)) // Return 0 if the N-th match was not found | ||
| /// Compiles the patterns seen so far, keyed by `(pattern, flags)`. |
There was a problem hiding this comment.
this looks similar to https://github.com/apache/datafusion/blob/998f534aafbf55c83daaa6fd4985ba143954b0e0/datafusion/functions/src/regex/mod.rs#L131-L130
Maybe we could (as a follow on PR) extract the common regexp caching functionality) -- I personally prefer this Cache pattern -- for use in the other functions with regular expressions
There was a problem hiding this comment.
Good point. I filed #23675. I will work on this once this PR merges.
…-datafusion-20260713-155538
|
I merged up to resolve the conflicts |
…pache#23620) ## Which issue does this PR close? N/A (found while reviewing test coverage for apache#23540) ## Rationale for this change While checking `cargo llvm-cov` coverage of `regexp_instr`, I found that the two `statement error` tests for `start < 1` in `regexp_instr.slt` never actually exercise the start validation. Coverage also showed two paths of `regexp_instr` untested anywhere: - the `N must be 1 or greater` error for `nth < 1` - looking up an already-compiled regex from the per-batch cache when the pattern column returns to a previously seen pattern (e.g. `['abc', 'def', 'abc']`). This path becomes more important with the memoization added in apache#23540. ## What changes are included in this PR? - Fix the two malformed `statement error` tests so the expected error is matched inline against the real message - Add a `statement error` test for `nth < 1` - Add a test with a pattern column that alternates between two regexes within a single batch ## Are these changes tested? Yes, this PR is only tests. ## Are there any user-facing changes? No Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23540 +/- ##
==========================================
- Coverage 80.66% 80.65% -0.01%
==========================================
Files 1087 1087
Lines 367409 367380 -29
Branches 367409 367380 -29
==========================================
- Hits 296367 296321 -46
- Misses 53390 53400 +10
- Partials 17652 17659 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4 participants
Which issue does this PR close?
N/A
Rationale for this change
Optimize existing expression.
What changes are included in this PR?
Removed per-batch default-array/Vec allocations, memoized the compiled regex across rows to skip HashMap hashing for literal patterns, and skipped the O(n) chars() scan on the default start=1 path.
Are these changes tested?
Existing tests.
Benchmark (criterion):
Full criterion output:
Are there any user-facing changes?
No